🛡️ Sentinel: Fix TOCTOU race condition in SSH key creation#27
🛡️ Sentinel: Fix TOCTOU race condition in SSH key creation#27
Conversation
- Use `umask 077` in a subshell when creating sensitive directories and files. - Ensure SSH private keys are created with 600 permissions atomically. - Add regression test `tests/test_ssh_creation.sh` to verify secure permissions. - Add security learning to `.jules/sentinel.md`. This prevents a brief window where the private key could be world-readable before `chmod` is applied. Co-authored-by: kidchenko <5432753+kidchenko@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis pull request enhances SSH key creation security by documenting a TOCTOU vulnerability pattern, implementing secure file creation with umask in the SSH setup script, and adding a test script to validate proper file permissions on restored SSH keys. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.jules/sentinel.md:
- Around line 1-4: Add a top-level H1 (e.g., a brief title) above the existing
"## 2025-02-11 - Secure File Creation with umask" heading, insert a blank line
immediately after that existing `##` heading to satisfy MD022, update the date
in the heading from 2025-02-11 to 2026-02-11, and reflow or break the long
sentences in the three lines describing Vulnerability/Learning/Prevention so
each line is ≤80 characters to satisfy MD013 (ensure punctuation and sentence
meaning remain intact while wrapping).
In `@tests/test_ssh_creation.sh`:
- Around line 60-78: Tests currently leak the temporary directory on failures
because the cleanup rm -rf "$TEST_HOME" runs only on success; add a trap early
in the script (after initial variable setup) that ensures TEST_HOME is removed
on EXIT (or on ERR/INT/TERM) so cleanup always runs, then remove the explicit rm
-rf "$TEST_HOME" at the end; reference the TEST_HOME, KEY_FILE, and SSH_DIR
variables when adding the trap to guarantee removal regardless of which
assertion fails.
- Around line 65-74: The test uses Linux-only stat flags to check file perms
(PERMS=$(stat -c "%a" "$KEY_FILE") and DIR_PERMS=$(stat -c "%a" "$SSH_DIR")),
which will fail on macOS; modify the permission-check logic in
tests/test_ssh_creation.sh to be portable by detecting the platform (e.g., via
uname) and using macOS-compatible stat -f "%Lp" for KEY_FILE and SSH_DIR or
fallback to a shell-based method (e.g., printf "%o" "$(stat -f "%Lp" ...)" or
using ls -l parsing) so the checks for KEY_FILE and SSH_DIR permissions work on
both Linux and macOS. Ensure the permission variables PERMS and DIR_PERMS are
set using the appropriate branch and keep the existing comparison logic that
expects "600" for KEY_FILE and "700" for SSH_DIR.
- Line 26: The mock script prints its arguments with echo "mock-op-called-with:
$@" which loses argument boundaries for values containing spaces; update the
echo invocation in tests/test_ssh_creation.sh to use quoted expansion ("$@") so
arguments like "SSH Key" remain a single parameter when printed by the mock.
🧹 Nitpick comments (1)
tools/setup-ssh-keys.sh (1)
164-166: Public key also created outside a umask subshell — acceptable but worth noting.The public key at line 165 is written with the default umask and then
chmod 644. Since the public key is not sensitive, there's no security issue here, but for consistency you could apply the same subshell pattern. Not blocking.
| ## 2025-02-11 - Secure File Creation with umask | ||
| **Vulnerability:** SSH private keys were created with default umask (often 022/002), making them world-readable for a brief window before `chmod` (TOCTOU race condition). | ||
| **Learning:** Redirection `>` in shell scripts respects current umask, creating files with potentially insecure permissions by default. `chmod` after creation is insufficient for high-security files. | ||
| **Prevention:** Wrap sensitive file creation commands in a subshell with `umask 077` (e.g., `(umask 077; command > file)`). This ensures atomic secure creation. |
There was a problem hiding this comment.
Fix markdown lint violations flagged by CI.
The linter reports several issues: missing top-level heading (MD041), no blank line after the ## heading (MD022), and lines 2–4 exceed the 80-character limit (MD013). Also, the date reads 2025-02-11 — should this be 2026-02-11?
Suggested structure
-## 2025-02-11 - Secure File Creation with umask
-**Vulnerability:** SSH private keys were created with default umask (often 022/002), making them world-readable for a brief window before `chmod` (TOCTOU race condition).
-**Learning:** Redirection `>` in shell scripts respects current umask, creating files with potentially insecure permissions by default. `chmod` after creation is insufficient for high-security files.
-**Prevention:** Wrap sensitive file creation commands in a subshell with `umask 077` (e.g., `(umask 077; command > file)`). This ensures atomic secure creation.
+# Sentinel Security Notes
+
+## 2026-02-11 - Secure File Creation with umask
+
+**Vulnerability:** SSH private keys were created with default umask
+(often 022/002), making them world-readable for a brief window
+before `chmod` (TOCTOU race condition).
+
+**Learning:** Redirection `>` in shell scripts respects current
+umask, creating files with potentially insecure permissions by
+default. `chmod` after creation is insufficient for high-security
+files.
+
+**Prevention:** Wrap sensitive file creation commands in a subshell
+with `umask 077` (e.g., `(umask 077; command > file)`).
+This ensures atomic secure creation.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2025-02-11 - Secure File Creation with umask | |
| **Vulnerability:** SSH private keys were created with default umask (often 022/002), making them world-readable for a brief window before `chmod` (TOCTOU race condition). | |
| **Learning:** Redirection `>` in shell scripts respects current umask, creating files with potentially insecure permissions by default. `chmod` after creation is insufficient for high-security files. | |
| **Prevention:** Wrap sensitive file creation commands in a subshell with `umask 077` (e.g., `(umask 077; command > file)`). This ensures atomic secure creation. | |
| # Sentinel Security Notes | |
| ## 2026-02-11 - Secure File Creation with umask | |
| **Vulnerability:** SSH private keys were created with default umask | |
| (often 022/002), making them world-readable for a brief window | |
| before `chmod` (TOCTOU race condition). | |
| **Learning:** Redirection `>` in shell scripts respects current | |
| umask, creating files with potentially insecure permissions by | |
| default. `chmod` after creation is insufficient for high-security | |
| files. | |
| **Prevention:** Wrap sensitive file creation commands in a subshell | |
| with `umask 077` (e.g., `(umask 077; command > file)`). | |
| This ensures atomic secure creation. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 160] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 3-3: Line length
.jules/sentinel.md:3:81 MD013/line-length Line length [Expected: 80; Actual: 199] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 2-2: Line length
.jules/sentinel.md:2:81 MD013/line-length Line length [Expected: 80; Actual: 170] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 1-1: First line in a file should be a top-level heading
.jules/sentinel.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "## 2025-02-11 - Secure File Cr..."] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md041.md
[failure] 1-1: Headings should be surrounded by blank lines
.jules/sentinel.md:1 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## 2025-02-11 - Secure File Creation with umask"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
🤖 Prompt for AI Agents
In @.jules/sentinel.md around lines 1 - 4, Add a top-level H1 (e.g., a brief
title) above the existing "## 2025-02-11 - Secure File Creation with umask"
heading, insert a blank line immediately after that existing `##` heading to
satisfy MD022, update the date in the heading from 2025-02-11 to 2026-02-11, and
reflow or break the long sentences in the three lines describing
Vulnerability/Learning/Prevention so each line is ≤80 characters to satisfy
MD013 (ensure punctuation and sentence meaning remain intact while wrapping).
| echo "mock-public-key-content" | ||
| fi | ||
| else | ||
| echo "mock-op-called-with: $@" >&2 |
There was a problem hiding this comment.
Unquoted $@ in mock script.
$@ should be "$@" to preserve argument boundaries containing spaces (e.g., a key name like "SSH Key").
- echo "mock-op-called-with: $@" >&2
+ echo "mock-op-called-with: $*" >&2🤖 Prompt for AI Agents
In `@tests/test_ssh_creation.sh` at line 26, The mock script prints its arguments
with echo "mock-op-called-with: $@" which loses argument boundaries for values
containing spaces; update the echo invocation in tests/test_ssh_creation.sh to
use quoted expansion ("$@") so arguments like "SSH Key" remain a single
parameter when printed by the mock.
| if [[ ! -f "$KEY_FILE" ]]; then | ||
| echo "FAIL: Key file not created" | ||
| exit 1 | ||
| fi | ||
|
|
||
| PERMS=$(stat -c "%a" "$KEY_FILE") | ||
| if [[ "$PERMS" != "600" ]]; then | ||
| echo "FAIL: Private key permissions are $PERMS (expected 600)" | ||
| exit 1 | ||
| fi | ||
|
|
||
| DIR_PERMS=$(stat -c "%a" "$SSH_DIR") | ||
| if [[ "$DIR_PERMS" != "700" ]]; then | ||
| echo "FAIL: SSH directory permissions are $DIR_PERMS (expected 700)" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "PASS: SSH key creation secure" | ||
| rm -rf "$TEST_HOME" |
There was a problem hiding this comment.
Temp directory leaked on test failure.
If any assertion fails (lines 61–74), the script exits before rm -rf "$TEST_HOME" on line 78. Use a trap to guarantee cleanup.
Suggested fix
Add near the top of the script (after line 5):
TEST_HOME=$(mktemp -d)
+trap 'rm -rf "$TEST_HOME"' EXITThen you can remove the explicit rm -rf at line 78.
🤖 Prompt for AI Agents
In `@tests/test_ssh_creation.sh` around lines 60 - 78, Tests currently leak the
temporary directory on failures because the cleanup rm -rf "$TEST_HOME" runs
only on success; add a trap early in the script (after initial variable setup)
that ensures TEST_HOME is removed on EXIT (or on ERR/INT/TERM) so cleanup always
runs, then remove the explicit rm -rf "$TEST_HOME" at the end; reference the
TEST_HOME, KEY_FILE, and SSH_DIR variables when adding the trap to guarantee
removal regardless of which assertion fails.
| PERMS=$(stat -c "%a" "$KEY_FILE") | ||
| if [[ "$PERMS" != "600" ]]; then | ||
| echo "FAIL: Private key permissions are $PERMS (expected 600)" | ||
| exit 1 | ||
| fi | ||
|
|
||
| DIR_PERMS=$(stat -c "%a" "$SSH_DIR") | ||
| if [[ "$DIR_PERMS" != "700" ]]; then | ||
| echo "FAIL: SSH directory permissions are $DIR_PERMS (expected 700)" | ||
| exit 1 |
There was a problem hiding this comment.
stat -c is Linux-only — this will fail on macOS.
The script under test references brew, strongly suggesting macOS as the primary target. On macOS, stat -c "%a" is unsupported; use stat -f "%Lp" instead.
Portable helper
+get_perms() {
+ if stat -c "%a" /dev/null &>/dev/null 2>&1; then
+ stat -c "%a" "$1"
+ else
+ stat -f "%Lp" "$1"
+ fi
+}
+
-PERMS=$(stat -c "%a" "$KEY_FILE")
+PERMS=$(get_perms "$KEY_FILE")
if [[ "$PERMS" != "600" ]]; then
echo "FAIL: Private key permissions are $PERMS (expected 600)"
exit 1
fi
-DIR_PERMS=$(stat -c "%a" "$SSH_DIR")
+DIR_PERMS=$(get_perms "$SSH_DIR")
if [[ "$DIR_PERMS" != "700" ]]; then
echo "FAIL: SSH directory permissions are $DIR_PERMS (expected 700)"
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PERMS=$(stat -c "%a" "$KEY_FILE") | |
| if [[ "$PERMS" != "600" ]]; then | |
| echo "FAIL: Private key permissions are $PERMS (expected 600)" | |
| exit 1 | |
| fi | |
| DIR_PERMS=$(stat -c "%a" "$SSH_DIR") | |
| if [[ "$DIR_PERMS" != "700" ]]; then | |
| echo "FAIL: SSH directory permissions are $DIR_PERMS (expected 700)" | |
| exit 1 | |
| get_perms() { | |
| if stat -c "%a" /dev/null &>/dev/null 2>&1; then | |
| stat -c "%a" "$1" | |
| else | |
| stat -f "%Lp" "$1" | |
| fi | |
| } | |
| PERMS=$(get_perms "$KEY_FILE") | |
| if [[ "$PERMS" != "600" ]]; then | |
| echo "FAIL: Private key permissions are $PERMS (expected 600)" | |
| exit 1 | |
| fi | |
| DIR_PERMS=$(get_perms "$SSH_DIR") | |
| if [[ "$DIR_PERMS" != "700" ]]; then | |
| echo "FAIL: SSH directory permissions are $DIR_PERMS (expected 700)" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In `@tests/test_ssh_creation.sh` around lines 65 - 74, The test uses Linux-only
stat flags to check file perms (PERMS=$(stat -c "%a" "$KEY_FILE") and
DIR_PERMS=$(stat -c "%a" "$SSH_DIR")), which will fail on macOS; modify the
permission-check logic in tests/test_ssh_creation.sh to be portable by detecting
the platform (e.g., via uname) and using macOS-compatible stat -f "%Lp" for
KEY_FILE and SSH_DIR or fallback to a shell-based method (e.g., printf "%o"
"$(stat -f "%Lp" ...)" or using ls -l parsing) so the checks for KEY_FILE and
SSH_DIR permissions work on both Linux and macOS. Ensure the permission
variables PERMS and DIR_PERMS are set using the appropriate branch and keep the
existing comparison logic that expects "600" for KEY_FILE and "700" for SSH_DIR.
1 participant
🛡️ Sentinel: [CRITICAL/HIGH] Fix SSH key creation permissions
🚨 Severity: HIGH
💡 Vulnerability: TOCTOU race condition in
tools/setup-ssh-keys.shwhere private keys were created with default umask (potentially world-readable) beforechmodwas applied.🎯 Impact: An attacker with local access could read the private key during the brief window between creation and permission change.
🔧 Fix: Wrapped file creation commands in a subshell with
umask 077to ensure atomic secure permissions.✅ Verification: Added
tests/test_ssh_creation.shwhich mocksopand verifies that files are created with 600 permissions even without explicitchmod.PR created automatically by Jules for task 2539125753142001948 started by @kidchenko
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation
Tests